Skip to content

Issue 52913: Fix determination of existing index names when dropping#6967

Merged
labkey-adam merged 18 commits into
developfrom
fb_drop_indices
Sep 3, 2025
Merged

Issue 52913: Fix determination of existing index names when dropping#6967
labkey-adam merged 18 commits into
developfrom
fb_drop_indices

Conversation

@labkey-adam
Copy link
Copy Markdown
Contributor

@labkey-adam labkey-adam commented Aug 26, 2025

Rationale

Reconcile and consolidate multiple index handling code paths into a single method that creates/drops indices on provisioned domains. And, especially, stop trying to guess an index's name when dropping it.

In the related PR, AbstractAuditTypeProvider.updateIndices() (which was already inspecting index meta data to determine indices to add/drop based on their characteristics) was taught to pass existing index names on to StorageProvisioner for dropping. This PR pushes the audit implementation into StorageProvisioner and switches callers of the redundant methods to use it.

The PR includes a bonus side quest that fixes domain URIs and domain URI resolution in a bunch of audit domains to ensure that we can use domain.getDomainKind() with confidence.

Related Pull Requests

Tasks 📍

  • Move AbstractAuditTypeProvider.updateIndices() code into StorageProvisioner
  • Fix Domains getting tagged with the wrong DomainKind
  • Junit tests that flag audit DomainKinds with overlapping prefixes
  • Switch all callers to use new StorageProvisioner.ensureTableIndices() method
  • Remove old code
  • Manual Testing / Verify Fix @labkey-chrisj
    • Ideally on a long-running database that's seen multiple upgrades, Before pulling down the feature branch and upgrading:
      • Review the "Admin Console - Audit Log" events for LDAP sync events, Study Security Escalations, Logged query events, Query export events, and EHR Security Escalations (if you have EHR built locally...)
      • Review in the Query Schema Browser queries in the "auditLog" schema corresponding to the above - PremiumLdapAuditEvent, StudySecurityEscalationEvent, LoggedQuery, QueryExportAuditEvent, and EHRSecurityEscalationEvent - Use Folder Filter -> All Folders to show all rows like the above do automatically
    • Sync FBs, build and start the server to run the exp upgrade
      • Re-review all "Admin Console - Audit Log" events and "auditLog" queries from above and verify the same row counts appear after upgrade. And no errors, of course.
    • On some lists and datasets, for some columns, check the Advanced checkbox that requires unique values. (Of course the values need to be unique or the table needs to be empty.) Check and save. Uncheck and save. Check and save. This will create and drop indices.
  • Needs Automation: No

Copy link
Copy Markdown
Contributor

@labkey-susanh labkey-susanh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR rationale says we stop guessing index names but there is still a TODO related to that. Still something you plan to do in this PR?

Comment thread api/src/org/labkey/api/audit/query/AbstractAuditDomainKind.java
Comment thread experiment/src/org/labkey/experiment/api/property/StorageProvisionerImpl.java Outdated
Comment thread api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java Outdated
Comment thread api/src/org/labkey/api/exp/property/DomainUtil.java Outdated
@labkey-adam
Copy link
Copy Markdown
Contributor Author

labkey-adam commented Sep 2, 2025

The PR rationale says we stop guessing index names but there is still a TODO related to that. Still something you plan to do in this PR?

I'm keen to get rid of this one last code path that guesses index names (on PostgreSQL... I'm not going to fight SQL Server at this point), but this PR has already taken longer than I wanted and I'd like it to bake a while before making more changes. And I'm anxious to get those SQL scripts and schema version bump merged. I've opened Issue 53838 to address this as a follow-on (and updated the comment in the method).

@labkey-adam labkey-adam merged commit 23ce916 into develop Sep 3, 2025
14 checks passed
@labkey-adam labkey-adam deleted the fb_drop_indices branch September 3, 2025 17:55
@labkey-adam labkey-adam mentioned this pull request Sep 5, 2025
2 tasks
@labkey-adam labkey-adam mentioned this pull request Sep 5, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants